[devant-migration] Add import config.toml function#606
[devant-migration] Add import config.toml function#606sm1990 merged 5 commits intowso2:devant-migrationfrom
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (18)
ipaas/src/components/SchemaConfigForm/schemaUtils.ts-51-51 (1)
51-51:⚠️ Potential issue | 🟡 MinorRegex sourced from caller-supplied
jsonPath.Static analysis flags the dynamic
RegExpconstructions on lines 56, 70, 81, and 92. Inputs are passed throughescapeRegex, so the inputs are treated as literal text and the patterns are bounded; the practical risk is low. Consider adding a brief comment nearescapeRegexdocumenting that callers must supply schema-derived paths (not arbitrary user input) so future contributors don't inadvertently bypass it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/schemaUtils.ts` at line 51, Add a short clarifying comment above the escapeRegex function explaining that it is intended to sanitize schema-derived jsonPath inputs (not arbitrary user input), that callers construct RegExp instances from these escaped jsonPath strings (e.g., the dynamic RegExp constructions that consume jsonPath), and that these patterns are therefore treated as literal text and bounded; this documents the intended usage and helps future contributors avoid bypassing the escaping.ipaas/src/components/SchemaConfigForm/schemaUtils.ts-168-176 (1)
168-176:⚠️ Potential issue | 🟡 MinorRemove dead code branches from
isNumberType.The
isNumberTypefunction checks for'integer'and'float', butJSONSchema['type']only includes'object' | 'array' | 'string' | 'number' | 'boolean' | 'null' | 'secret'. Since callers passschema.typedirectly (e.g., in ObjectElement.tsx wherepropType = prop.type), the branches for'integer'and'float'are unreachable. Either widenJSONSchema['type']to include these values (if the backend can emit them) or remove the dead branches fromisNumberType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/schemaUtils.ts` around lines 168 - 176, The isNumberType function contains unreachable checks for 'integer' and 'float'; update it to only check for 'number' (e.g., change isNumberType to return type === 'number'), and ensure related utilities (isBaseType and typeDisplayName) continue to call isNumberType unchanged so behavior remains consistent; if the backend can actually emit 'integer'/'float' instead, instead widen the JSONSchema['type'] union to include those strings and update any type annotations accordingly—pick one approach and apply it consistently across isNumberType, isBaseType, and typeDisplayName.ipaas/src/components/SchemaConfigForm/schemaUtils.ts-77-100 (1)
77-100:⚠️ Potential issue | 🟡 MinorBroaden the character class in regex patterns for map key extraction.
The patterns
\.(\w+)inextractAllMapKeySetandextractUniqueMapKeySetonly match[A-Za-z0-9_], excluding hyphens and other characters commonly found in configuration keys (e.g., TOML and JSON schema property names). Keys likeconfig.api-keywill silently fail pattern matching and be skipped. Use a broader pattern like[^.]+to capture all characters except the path delimiter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/schemaUtils.ts` around lines 77 - 100, In extractAllMapKeySet and extractUniqueMapKeySet update the RegExp from `\.(\w+)` to `\.([^.]+)` so keys with hyphens or other non-word characters (anything except a dot) are matched; also in extractUniqueMapKeySet, when you use the regex match, add the captured group (match[1]) to uniqueKeySet rather than match[0] so only the key portion is stored. Ensure you keep the existing basePath/escapeRegex handling and only change the pattern and the value added to the set.ipaas/src/components/SchemaConfigForm/FormElements/BaseElement.tsx-88-92 (1)
88-92:⚠️ Potential issue | 🟡 MinorTruthiness check on
valueMap.get(jsonPath)may misclassify legitimate values.
isExistingSensitiveConfigflips totruewhenever the stored value is any falsy primitive ('',0,false,null,undefined). For a sensitivestringfield that legitimately holds an empty string, or anumberfield holding0, the UI will switch to the masked "Update Sensitive Content" view instead of showing the editable input. Consider checking explicitly for the "stored-but-not-echoed-back" sentinel (e.g.valueMap.get(jsonPath) === nullor=== undefined) rather than negating any falsy value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/BaseElement.tsx` around lines 88 - 92, The truthiness check in the useMemo that defines isExistingSensitiveConfig causes legitimate falsy values to be treated as "masked"; update the predicate inside the useMemo (which references isSensitive, valueMap, jsonPath and produces isExistingSensitiveConfig) to only treat values that are truly absent/placeholder (e.g. valueMap.get(jsonPath) === null || valueMap.get(jsonPath) === undefined) as “stored-but-not-echoed-back” instead of negating any falsy value; keep the useEffect that reacts to isExistingSensitiveConfig and the call to setShowSensitiveInput intact so the UI only switches to the masked view for null/undefined sentinel values.ipaas/src/components/EnvironmentCard/EnvironmentCardHeader.tsx-81-81 (1)
81-81:⚠️ Potential issue | 🟡 Minor
buildDisabledwill essentially never disable theTestbutton.
buildDisabled = isBuildInProgress && (!hasDeployment || isAutomation). TheTestbutton only renders whenhasDeployment && onTest && !envCriticaland is part of theisGenericServiceblock (not automation), so both branches of(!hasDeployment || isAutomation)evaluatefalsewhenever the Test button is shown. As a result,disabled={buildDisabled}on Line 125 is effectively a no-op.If the intent is to also block
Testduring an in-progress build for generic services, consider gating it directly onisBuildInProgress(or splitting the predicate). Otherwise, theTestdisable can be removed.Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/EnvironmentCardHeader.tsx` at line 81, Summary: buildDisabled is currently computed as "isBuildInProgress && (!hasDeployment || isAutomation)" which never becomes true when the Test button is rendered, so the disabled prop on the Test button is a no-op. Fix: either remove the disabled={buildDisabled} from the Test button or change the predicate so Test is actually blocked during builds—for example keep the existing buildDisabled for general buttons (rename to buildDisabledGeneral) and add buildDisabledTest = isBuildInProgress (or simply set buildDisabled = isBuildInProgress) and use buildDisabledTest on the Test button. Update references to buildDisabled in EnvironmentCardHeader (and where Test renders: the isGenericService block guarded by hasDeployment && onTest && !envCritical) so semantics are correct.ipaas/src/components/SchemaConfigForm/FormElements/ObjectElementContainer.tsx-77-101 (1)
77-101:⚠️ Potential issue | 🟡 MinorHeader lacks keyboard activation and the delete
aria-labelmismatches the element type.Two minor accessibility concerns:
- The collapsible header (
Stackon Lines 77–82) only handlesonClick. Keyboard users cannot expand/collapse the section. Addrole="button",tabIndex={0}, and anonKeyDownhandler forEnter/Space(or render the header as a<button>).- On Line 95, the delete button is labeled
"delete array element"even though this container also wraps object entries (e.g. when reached fromObjectElement/PopOverComponent/MapElement). Use a generic label such as"delete entry", or derive it fromtitle/type.♻️ Suggested adjustments
<Stack direction="row" alignItems="center" justifyContent="space-between" + role="button" + tabIndex={0} onClick={() => setOpen((p) => !p)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setOpen((p) => !p); + } + }} sx={{ px: 1.5, py: 0.75, cursor: 'pointer', userSelect: 'none', bgcolor: 'action.hover', borderBottom: open ? '1px solid' : 'none', borderColor: 'divider' }}> ... - aria-label="delete array element"> + aria-label={`delete ${title || 'entry'}`}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/ObjectElementContainer.tsx` around lines 77 - 101, The header Stack in ObjectElementContainer only toggles via onClick and lacks keyboard accessibility; add role="button", tabIndex={0} and an onKeyDown handler that calls setOpen((p)=>!p) when Enter or Space is pressed (or render it as a native button) so keyboard users can expand/collapse; also update the IconButton delete control (Trash2) to use a generic aria-label like "delete entry" or derive it from title/type instead of "delete array element", keeping the existing e.stopPropagation() and onDelete(jsonPath) behavior.ipaas/src/pages/Component.tsx-76-85 (1)
76-85:⚠️ Potential issue | 🟡 MinorBuild-progress flag from the first deployment entry gates actions for all environments.
useDeploymentStatusreturns an array of deployment entries. Line 77 derivesisBuildInProgressfrom onlybuildDeployments[0]?.status, and this flag is passed to everyEnvironmentcard on line 139.The impact:
Incorrect gating: In
EnvironmentCardHeader(line 81),buildDisabled = isBuildInProgress && (!hasDeployment || isAutomation)disables build actions for all environments whenever the first deployment is in progress, even if other environments are not actively building.Fragile transition tracking: The
useEffecttransition detection relies onprevBuildStatusRef.currenttrackingbuildDeployments[0]?.status. If the array is reordered across refetches or entries are added/removed, status transitions may be missed or incorrectly detected.Consider passing per-environment build status to each
Environmentcard, or if a global flag is intentional, derive it from all deployments (e.g.,buildDeployments.some(d => d.status === 'in_progress')) and track transitions using a set of active deployment IDs rather than a single ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/pages/Component.tsx` around lines 76 - 85, The current logic uses buildDeployments[0]?.status (isBuildInProgress) and a single prevBuildStatusRef to gate builds and detect transitions; instead compute per-environment build state or a correct global state and track active IDs: change uses of isBuildInProgress to either (a) derive a per-environment flag by matching deployment entries to the Environment component’s environment id (pass that flag into EnvironmentCardHeader for buildDisabled), or (b) if a global lock is intended, replace buildDeployments[0]?.status with buildDeployments.some(d => d.status === 'in_progress') and replace prevBuildStatusRef (string) with a Set of active deployment ids to detect transitions (compare previous Set to current to know which deployments completed) so EnvironmentCardHeader.buildDisabled and the useEffect transition logic use accurate status data; update references to isBuildInProgress, prevBuildStatusRef, useDeploymentStatus, and EnvironmentCardHeader accordingly.ipaas/src/components/SchemaConfigForm/ConfigForm.tsx-154-154 (1)
154-154:⚠️ Potential issue | 🟡 Minor
String.replace('.', '/')only replaces the first dot.If a section path contains multiple
.segments (e.g.,a.b.c), only the first becomes/, yieldinga/b.c. Likely intent is to replace all separators for display:Proposed fix
- <SectionAccordion key={`${path}-${requiredOnly ? 'required' : 'optional'}`} title={path.replace('.', '/')} defaultExpanded contentSx={{ px: 2, pt: 1, pb: 1 }} subAccordion> + <SectionAccordion key={`${path}-${requiredOnly ? 'required' : 'optional'}`} title={path.replaceAll('.', '/')} defaultExpanded contentSx={{ px: 2, pt: 1, pb: 1 }} subAccordion>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/ConfigForm.tsx` at line 154, The display transformation in the SectionAccordion usage uses path.replace('.', '/'), which only swaps the first dot; update the transformation so all dots become slashes (e.g., use String.prototype.replaceAll with '.' or perform a split on '.' and join with '/' or use a global regex replace) when computing the title for SectionAccordion to ensure paths like "a.b.c" render as "a/b/c".ipaas/src/components/SchemaConfigForm/FormElements/MapElement.tsx-81-81 (1)
81-81:⚠️ Potential issue | 🟡 Minor
valueSchema.requiredis being overridden with the parent map schema'srequired.
schema.required(per JSON Schema semantics for an object withadditionalProperties) describes which keys must exist on the parent object, not which sub-fields are required inside each map value. SpreadingadditionalPropertiesand then assigningrequired: schema.requiredmixes those two scopes and may surface misleading required-field markers inside the value editor (BaseElement/ObjectElementContainer/etc.).If the intent was to forward the property-level required marker to control whether the entry's value editor renders as required, consider passing that through an explicit prop instead of mutating the value-schema's
required:Proposed fix
- const valueSchema = schema.additionalProperties && typeof schema.additionalProperties === 'object' ? { ...(schema.additionalProperties as JSONSchema), required: schema.required } : undefined; + const valueSchema = + schema.additionalProperties && typeof schema.additionalProperties === 'object' + ? (schema.additionalProperties as JSONSchema) + : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/MapElement.tsx` at line 81, The current code mutates the value schema by assigning required: schema.required when building valueSchema (variable valueSchema using schema.additionalProperties), which incorrectly mixes parent-object key requirements into the child value's schema; instead, keep valueSchema as a pure copy of schema.additionalProperties (do not set required on it) and pass a separate boolean/prop to the value editor (e.g., valueIsRequired or entryRequired) derived from schema.required if you need to indicate the parent-level “key is required” to the editor; update the MapElement rendering code to send that explicit prop to the value editor (the component that renders BaseElement/ObjectElementContainer for map values) rather than mutating valueSchema.ipaas/src/components/SchemaConfigForm/FormElements/PopOverComponent.tsx-127-222 (1)
127-222:⚠️ Potential issue | 🟡 MinorInconsistent forwarding of linking/configGroups props across branches.
BaseElement(Line 110-115) andAnyOfElement(Line 180-185) receiveallowLinking,configGroups,linkingMap, andsetLinkingMap, but theMapElement,ObjectElementContainer, andArrayElementbranches do not. This means linking and config-group features will silently not propagate when those branches render. Either forward the props uniformly or add a comment justifying the asymmetry.Proposed fix (apply to MapElement, ObjectElementContainer, and ArrayElement branches)
<MapElement key={key} ... isRequiredAtRequiredLevel={isRequiredAtRequiredLevel} + allowLinking={allowLinking} + configGroups={configGroups} + linkingMap={linkingMap} + setLinkingMap={setLinkingMap} sensitiveMap={sensitiveMap} setSensitiveMap={setSensitiveMap} />(Repeat for
ObjectElementContainerLine 148 andArrayElementLine 201.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/PopOverComponent.tsx` around lines 127 - 222, The MapElement, ObjectElementContainer, and ArrayElement branches are not receiving the linking-related props (allowLinking, configGroups, linkingMap, setLinkingMap) while BaseElement and AnyOfElement do; forward those props into MapElement, ObjectElementContainer, and ArrayElement (add allowLinking={allowLinking} configGroups={configGroups} linkingMap={linkingMap} setLinkingMap={setLinkingMap}) so linking and config-group behavior is consistent across branches, or add a brief inline comment in each branch explaining intentional omission if asymmetry is required.ipaas/src/components/SchemaConfigForm/FormElements/ObjectElement.tsx-80-196 (1)
80-196:⚠️ Potential issue | 🟡 MinorInconsistent
isRequiredderivation across branches.For each property key, the required-flag passed to children differs by branch:
MapElement(Line 90): parent'sisRequired- nested
ObjectElement(Line 115): parent'sisRequiredBaseElement(Line 139):schema.required?.includes(key)(correct per JSON Schema semantics)ArrayElement(Line 152-173): not passed at allAnyOfElement(Line 186): parent'sisRequiredJSON Schema's
requiredarray on the current object schema lists which keys are required, so the per-child required flag should be derived fromschema.required?.includes(key)— as already done forBaseElement. Consider unifying:Proposed fix
const generatedJsonPath = generateObjectJsonPath(jsonPath, key); const nestedLabel = generatedNestedLabel(title, subTitle); + const isKeyRequired = schema.required?.includes(key);Then pass
isRequired={isKeyRequired}consistently toMapElement, the recursiveObjectElement,ArrayElement, andAnyOfElement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/ObjectElement.tsx` around lines 80 - 196, Compute a single isKeyRequired variable once (e.g. const isKeyRequired = schema.required?.includes(key)) and use it for all child components instead of the inconsistent usages now; update MapElement, the nested ObjectElement invocation, ArrayElement, AnyOfElement (and keep BaseElement as-is) to accept isRequired={isKeyRequired} so ArrayElement gets the required flag and MapElement/ObjectElement/AnyOfElement no longer rely on the parent's isRequired or omit it; reference symbols: MapElement, ObjectElement, ArrayElement, AnyOfElement, BaseElement, schema, prop, key.ipaas/src/components/EnvironmentCard/index.tsx-110-111 (1)
110-111:⚠️ Potential issue | 🟡 MinorThe hook prevents execution when no deployment exists—verify this is intended.
useSchemaConfighas an explicitenabledgate requiringcommitHashto be truthy. WhendeployedCommitShaisundefined(no successful build/deployment yet), the query is disabled and does not execute. This meansschemaConfigremainsundefineduntil the first deployment completes. Confirm this UX is acceptable—users cannot see configuration requirements until after first deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/index.tsx` around lines 110 - 111, The query for schema config is currently disabled whenever deployedCommitSha is falsy (no deployment), so confirm desired UX and either (A) allow the query to run before first deployment by changing how useSchemaConfig is called here—pass a placeholder/empty string for commitHash or add a conditional that calls useSchemaConfig with isAutomation ? deployedCommitSha ?? '' : ''—or (B) change the hook’s enabled logic to rely on other identifiers (projectId/componentId/envTemplateId/versionId) instead of requiring commitHash; update references to deployedCommitSha, useSchemaConfig, and envDeployment accordingly so schemaConfig is fetched when you intend it to be visible prior to a first deployment.ipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsx-161-165 (1)
161-165:⚠️ Potential issue | 🟡 MinorPending
setTimeoutis not cleared on unmount.
handleCloseschedulesresetLocalValueMap500 ms later. If the component unmounts (or closes again) in that window, the callback still fires and triggers state updates on an unmounted instance. Track the timer id and clear it in a cleanupuseEffect, or guard with a mounted ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsx` around lines 161 - 165, handleClose currently calls setTimeout(() => resetLocalValueMap(valueMap, validationMap), 500) which can fire after the component unmounts; modify the component to store the timer id returned by setTimeout (e.g., in a ref like timeoutRef) when setting the timeout in handleClose and add a useEffect cleanup that clears the timeout (clearTimeout(timeoutRef.current)) or check a mounted ref before calling resetLocalValueMap; ensure you cancel the pending timer when closing or unmounting to avoid calling resetLocalValueMap on an unmounted component and reference the existing symbols handleClose, resetLocalValueMap, valueMap, validationMap, setAnchorEl, setOpen, and the new timeoutRef/mountedRef in your changes.ipaas/src/components/SchemaConfigForm/tomlUtils.ts-40-50 (1)
40-50:⚠️ Potential issue | 🟡 MinorInline table parsing splits on every comma.
tableContent.split(',')doesn't account for commas inside quoted strings or nested arrays/inline tables, so values like{ a = "x,y", b = [1, 2] }are mis-parsed. Consider reusing the depth/string-aware splitter already used for arrays at lines 56–78.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/tomlUtils.ts` around lines 40 - 50, The inline-table parsing block that slices tableContent and uses tableContent.split(',') incorrectly splits on commas inside quoted strings or nested structures; replace that naive split with the same depth-and-quote-aware splitter used for array parsing (the logic used later in the file for arrays that tracks quote state and bracket/brace depth) so you produce correct pairs for values like `{ a = "x,y", b = [1, 2] }`; then keep the existing pair processing (const [key, ...valueParts] = pair.split('='), result[key.trim()] = parseTomlValue(...)) to parse each key/value.ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx-686-716 (1)
686-716:⚠️ Potential issue | 🟡 MinorMount path/
mountedAsderived from a single cert's path.
lastSlashis computed once fromcfgs[0].key(line 699) and then reused for every key in the same group on lines 700 and 708. If sibling configs in the same group have different prefixes, the resultingmountPathandmountedAswill be wrong for all but the first. Recompute the slash position percfgs[i]when derivingmountedAs, and consider validating that all members of a group share the same directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx` around lines 686 - 716, The grouping logic in ConfigureDrawer's useEffect builds mountPath and mountedAs using lastSlash derived only from cfgs[0].key, which breaks when sibling configs in the same group have different directories; fix by computing the slash position per config inside the cfgs.map callback (use each cfg.key to derive its own lastSlash and mountedAs) and either validate that all cfgs in a group share the same directory before setting a single mountPath (e.g., compute mountPath from the first and throw/log/normalize if others differ) or derive mountPath consistently (e.g., use the common directory computed across cfgs); update the mapping code that constructs certs/keys (variables: byGroup, cfgs, fullKey/lastSlash/mountPath, mountedAs) and keep setting setLinkedCerts/certSeededRef behavior unchanged.ipaas/src/components/SchemaConfigForm/tomlUtils.ts-116-127 (1)
116-127:⚠️ Potential issue | 🟡 MinorKey/value regex over-matches and ignores quoted keys.
^([^=]+)=(.+)$will (a) treatkey = "a=b"correctly because the second group is greedy, but (b) silently fail for TOML quoted keys containing=(e.g."k=1" = "v"), and (c) include trailing inline comments (key = 1 # note) as part of the value. Consider stripping inline comments and tightening the key parsing to TOML grammar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/tomlUtils.ts` around lines 116 - 127, The key/value matching currently uses a greedy regex that fails for quoted keys containing '=' and leaves inline comments on values; update the parsing in tomlUtils.ts to locate the first '=' character that's not inside a quoted string (i.e., scan characters and track quote state) then split into key and rawValue, trim both, support quoted keys by unwrapping surrounding quotes (handle both "..." and '...') and finally strip inline comments from rawValue only when the '#' is not inside quotes before calling parseTomlValue; apply these changes where keyValueMatch is used (refer to parseTomlValue, currentArrayTable, arrayTables, currentSection, and result) so array/table and section handling remains identical.ipaas/src/components/SchemaConfigForm/tomlUtils.ts-297-309 (1)
297-309:⚠️ Potential issue | 🟡 MinorRegex compiled from external input.
new RegExp(^${schemaWithWildcard}$)is built from schema-derived strings without escaping. Any property name containing regex metacharacters (.,+,(, etc.) will either match incorrectly or throw at runtime. Escape non-wildcard segments before composing the pattern, and consider precomputing the regexes once pervalidKeysto avoid repeated allocation per TOML key.🛠️ Suggested fix sketch
- const schemaWithWildcard = targetSchema.replace(/\[\*\]/g, '\\[\\d+\\]'); - const regexPattern = new RegExp(`^${schemaWithWildcard}$`); + const escaped = targetSchema.replace(/[.+?^${}()|[\]\\]/g, '\\$&'); + const schemaWithWildcard = escaped.replace(/\\\[\\\*\\\]/g, '\\[\\d+\\]'); + const regexPattern = new RegExp(`^${schemaWithWildcard}$`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/tomlUtils.ts` around lines 297 - 309, The regex in matchesWithStripping (inside checkKeyMatch) is constructed from schema-derived strings (schemaWithWildcard) without escaping, which can cause incorrect matches or runtime errors; update the logic to first escape all regex metacharacters in the schema segments except the intended wildcard token ([*]) before replacing that token with the literal digit-group pattern (\\[\\d+\\]), and replace the inline new RegExp creation with precomputed RegExp objects (e.g., build a map of schemaKey -> RegExp once outside matchesWithStripping) so checkKeyMatch uses the cached RegExp instead of allocating per tomlKey; refer to matchesWithStripping, checkKeyMatch, schemaWithWildcard and the new RegExp(...) usage when making these changes.ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx-916-920 (1)
916-920:⚠️ Potential issue | 🟡 MinorAdd a safeguard for required fields that don't seed validation entries.
On first open before any field interaction, if a required field is complex (array, anyOf, map/object with additionalProperties), its validation entry won't exist in the map.
Array.from(validationMap.values()).some((v) => !v)returnsfalsewhen the map is empty, allowing step progression without validation. AddvalidationMap.size === 0as an additional gate tonextDisabled:const nextDisabled = step === 1 ? (hasValidationErrors || validationMap.size === 0 || isLoading) : isApplying;This ensures the form cannot advance when required fields haven't yet populated the validation map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx` around lines 916 - 920, The step advancement logic lets the form proceed when validationMap is empty (no seeded entries for complex required fields); update the nextDisabled calculation used with step, hasValidationErrors, and isLoading so it also treats an empty validationMap as invalid — i.e., when step === 1 disable Next if validationMap.size === 0 in addition to hasValidationErrors or isLoading; adjust the expression that computes nextDisabled (referencing validationMap, hasValidationErrors, step, and isLoading) to include this extra guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f85ca9f-5dc4-489d-aeea-d5d586064112
📒 Files selected for processing (41)
ipaas/src/api/apim.tsipaas/src/api/mutations.tsipaas/src/api/queries.tsipaas/src/components/AutomationExecutions.tsxipaas/src/components/Deploy/BuildArea.tsxipaas/src/components/Deploy/BuildAreaImageDrawer.tsxipaas/src/components/Deploy/BuildImageCard.tsxipaas/src/components/Deploy/DeployEnvironmentCard/DeployEnvironmentCard.tsxipaas/src/components/Deploy/DeployEnvironmentCard/DeployEnvironmentCardBody.tsxipaas/src/components/Deploy/DeployEnvironmentCard/DeployEnvironmentCardHeader.tsxipaas/src/components/Deploy/DeployEnvironmentCard/DeploymentHistoryDrawer.tsxipaas/src/components/Deploy/DeployEnvironmentCard/EndpointsDrawer.tsxipaas/src/components/Deploy/EndpointConfigDrawer.tsxipaas/src/components/Deploy/TagInput.tsxipaas/src/components/EnvironmentCard/ConfigureDrawer.tsxipaas/src/components/EnvironmentCard/EndpointCard.tsxipaas/src/components/EnvironmentCard/EnvironmentCardHeader.tsxipaas/src/components/EnvironmentCard/ManageDrawer.tsxipaas/src/components/EnvironmentCard/index.tsxipaas/src/components/SchemaConfigForm/ConfigForm.tsxipaas/src/components/SchemaConfigForm/FormElements/AnyOfElement.tsxipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsxipaas/src/components/SchemaConfigForm/FormElements/BaseElement.tsxipaas/src/components/SchemaConfigForm/FormElements/ConfigElement.tsxipaas/src/components/SchemaConfigForm/FormElements/MapElement.tsxipaas/src/components/SchemaConfigForm/FormElements/ObjectElement.tsxipaas/src/components/SchemaConfigForm/FormElements/ObjectElementContainer.tsxipaas/src/components/SchemaConfigForm/FormElements/PopOverComponent.tsxipaas/src/components/SchemaConfigForm/index.tsipaas/src/components/SchemaConfigForm/schemaUtils.tsipaas/src/components/SchemaConfigForm/tomlUtils.tsipaas/src/hooks/usePrebuiltIntegrations.tsipaas/src/hooks/useProjectId.tsipaas/src/hooks/useSamples.tsipaas/src/pages/Component.tsxipaas/src/pages/Deploy.tsxipaas/src/pages/OrgBuild.tsxipaas/src/pages/OrgDeploy.tsxipaas/src/pages/ProjectBuild.tsxipaas/src/pages/ProjectDeploy.tsxipaas/src/utils/componentType.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
ipaas/src/components/EnvironmentCard/ManageDrawer.tsx (2)
358-362: Extract therequestCountparsing into a small helper.The inline IIFE
((_rc) => (isNaN(_rc) ? -1 : _rc))(Number(op.throttlingPolicy))duplicates the parsing logic introduced at Line 345–346. A shared helper would be clearer and keep the two call sites in sync.Proposed refactor
+ const parseRequestCount = (value: string | number | null | undefined) => { + const n = Number(value); + return Number.isNaN(n) ? -1 : n; + }; const settingsKey = (savedApi.revisionedApiId as string | null | undefined) || savedApi.name; - const apiLevelRc = Number(state.apiLevelPolicy.requestCount); - const apiLevelThrottle = state.rateLimitLevel === 'API_LEVEL' ? { requestCount: isNaN(apiLevelRc) ? -1 : apiLevelRc, unit: state.apiLevelPolicy.timeUnit } : null; + const apiLevelThrottle = + state.rateLimitLevel === 'API_LEVEL' + ? { requestCount: parseRequestCount(state.apiLevelPolicy.requestCount), unit: state.apiLevelPolicy.timeUnit } + : null; @@ - throttlingLimit: { requestCount: ((_rc) => (isNaN(_rc) ? -1 : _rc))(Number(op.throttlingPolicy)), unit: 'MINUTE' }, + throttlingLimit: { requestCount: parseRequestCount(op.throttlingPolicy), unit: 'MINUTE' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/ManageDrawer.tsx` around lines 358 - 362, Duplicate inline parsing of throttlingPolicy is used inside the updatedOperations.map (building throttlingLimit.requestCount) and earlier at the other call site; extract that logic into a small helper (e.g., parseRequestCount or parseThrottlingPolicy) that accepts the raw value (op.throttlingPolicy) or Number(...), returns -1 when NaN or the numeric value otherwise, and replace both inline IIFEs with a call to this helper; update references in the mapping that set throttlingLimit.requestCount and the earlier use (around the code that previously had the same Number(...) parsing) to call the new helper (keep helper name unique to find it easily).
363-363:Number(state.timeout) || undefineddrops a configured0.
validateTimeoutalready rejects non-positive values before this code runs, so this is currently safe. Consider aligning with the explicit-NaN pattern used elsewhere for consistency, e.g.Number.isFinite(n) && n > 0 ? n : undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/ManageDrawer.tsx` at line 363, The assignment resiliency: Number(state.timeout) || undefined will coerce a valid 0 to undefined; update the resiliency calculation to use an explicit finite-number check consistent with other code (e.g., use Number.isFinite on the parsed value and ensure > 0 if that’s desired) so that resiliency is set only when the timeout parses to a valid number; locate the resiliency property assignment in ManageDrawer (and reference validateTimeout to keep behavior consistent) and replace the || undefined pattern with the explicit-NaN/finite check used elsewhere.ipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsx (1)
162-166: Pending timeout may run after unmount.
setTimeout(() => resetLocalValueMap(...), 500)is not cancelled if the component unmounts (orvalueMap/validationMapchange) before it fires, which can trigger a state update on an unmounted component. Track the timeout id and clear it in a cleanup effect or inhandleClose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsx` around lines 162 - 166, handleClose currently schedules resetLocalValueMap via setTimeout and doesn't cancel it, which can run after unmount and cause state updates on an unmounted component; fix by storing the timeout id when calling setTimeout in handleClose (e.g., let timeoutId = window.setTimeout(...)) and clear it with clearTimeout(timeoutId) before creating a new timeout and in a useEffect cleanup that runs on unmount or when valueMap/validationMap change; ensure references to handleClose, resetLocalValueMap, valueMap, validationMap and the stored timeout id are used so the pending timeout is always cancelled.ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx (1)
489-499:envIdanddeploymentTrackIdprops are unused inCertificateMountStep.They are immediately renamed to
_envId/_deploymentTrackId. Either drop them from the prop type and the call site at line 922, or use them (e.g., for the cert-mappings query) to keep the API surface honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx` around lines 489 - 499, CertificateMountStep declares props envId and deploymentTrackId but immediately renames them to _envId/_deploymentTrackId and never uses them; either remove these two props from CertificateMountStepProps and stop passing them from callers (update every invocation that supplies envId/deploymentTrackId to no longer pass those args) or actually use them (e.g., include envId and deploymentTrackId in the cert-mappings query inside CertificateMountStep so the variables are referenced rather than renamed). Update the prop interface (CertificateMountStepProps), the function signature for CertificateMountStep, and all call sites that instantiate <CertificateMountStep ... /> to keep the API consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx`:
- Around line 695-711: In ConfigureDrawer.tsx inside the certs mapping, change
the slash-handling to use lastSlash >= 0 (not > 0) and treat root-mounted keys
explicitly so we don't fallback to '/certs' while keeping the original key as
mountedAs; specifically, compute lastSlash = fullKey.lastIndexOf('/') and then
set mountPath to fullKey.substring(0, lastSlash) when lastSlash >= 0 (use '/' if
that substring is empty) otherwise default to '/certs', and compute mountedAs
using c.key.substring(lastSlash + 1) only when lastSlash >= 0 (otherwise use
c.key); update the mapping in the certs creation (variables fullKey, lastSlash,
mountPath, mountedAs) accordingly to avoid producing duplicated slashes on save.
- Around line 685-716: The current reset logic doesn't clear linkedCerts, so
stale selections persist across close/open; update the component's reset effect
(the effect that currently resets other state when the drawer is closed) to call
setLinkedCerts([]) and reset certSeededRef.current = false when open becomes
true/when closing so that on open linkedCerts is cleared and the existing
seeding effect (which builds certs from existingCertMappings and uses
certSeededRef and setLinkedCerts) can repopulate it only when there are backend
mappings; ensure you only set linkedCerts to empty on open/reset and let the
seeding use its existing certs.length > 0 logic to populate.
In `@ipaas/src/components/SchemaConfigForm/tomlUtils.ts`:
- Around line 232-258: getAllSchemaKeys currently treats an object with
properties and additionalProperties as mutually exclusive, so keys under
additionalProperties are skipped; update collectKeys (inside getAllSchemaKeys)
so when property.type === 'object' you always enumerate named property keys (if
property.properties) and also handle property.additionalProperties (if truthy)
by adding the parent fullPath to keys and, when additionalProperties is an
object with properties, recursively calling collectKeys on it with a marker like
`${fullPath}.*`; likewise ensure array handling still recurses into items when
present—i.e., do not use else-if chains that prevent handling both properties
and additionalProperties in the same branch.
- Around line 40-50: The inline-table parser currently uses
tableContent.split(',') which breaks on commas inside quoted strings or nested
structures; replace this with the same char-by-char depth-and-string tracking
used by the array branch (around lines 60-78) to produce correct "pairs"
(iterate characters, track inString and nesting depth, collect segments when
hitting a top-level comma). Also ensure splitting each pair into key and value
uses the first '=' found that is not inside a string or nested structure (i.e.,
split on the first top-level '='), then trim and pass the value part to
parseTomlValue; update the code in the inline table branch (the block creating
pairs/result) accordingly.
- Around line 86-138: The parser splits by lines so triple-quoted multiline
values are lost; update parseToml to detect when a key's value starts with
triple quotes (''' or """) and does not end on the same line, then accumulate
subsequent lines (advancing lineNumber and skipping normal parsing) until the
matching closing triple quotes are found, join those lines into the full string
and pass that to parseTomlValue; adjust the key/value handling in parseToml
(around keyValueMatch handling and array table assignment) to support this
accumulation and still use parseTomlValue's triple-quoted branch, or
alternatively add a validation in isValidTomlFile to explicitly reject multiline
triple-quoted strings if you choose not to support them.
- Around line 297-309: The regex built in matchesWithStripping (inside
checkKeyMatch) currently injects schema text directly which lets regex
metacharacters like . + ? etc. alter matching; fix by escaping all regex
metacharacters in targetSchema before converting the array-wildcard token: use a
safe escape (e.g., replace /[.*+?^${}()|[\]\\]/g -> '\\$&') on targetSchema,
then replace the escaped token for [*] with the intended '\\[\\d+\\]' sequence,
and finally build the anchored RegExp(`^...$`) as before; update the
schemaWithWildcard/regexPattern creation in matchesWithStripping/checkKeyMatch
to use the escaped-and-replaced string so schema names like "service.name" match
literally.
---
Nitpick comments:
In `@ipaas/src/components/EnvironmentCard/ConfigureDrawer.tsx`:
- Around line 489-499: CertificateMountStep declares props envId and
deploymentTrackId but immediately renames them to _envId/_deploymentTrackId and
never uses them; either remove these two props from CertificateMountStepProps
and stop passing them from callers (update every invocation that supplies
envId/deploymentTrackId to no longer pass those args) or actually use them
(e.g., include envId and deploymentTrackId in the cert-mappings query inside
CertificateMountStep so the variables are referenced rather than renamed).
Update the prop interface (CertificateMountStepProps), the function signature
for CertificateMountStep, and all call sites that instantiate
<CertificateMountStep ... /> to keep the API consistent.
In `@ipaas/src/components/EnvironmentCard/ManageDrawer.tsx`:
- Around line 358-362: Duplicate inline parsing of throttlingPolicy is used
inside the updatedOperations.map (building throttlingLimit.requestCount) and
earlier at the other call site; extract that logic into a small helper (e.g.,
parseRequestCount or parseThrottlingPolicy) that accepts the raw value
(op.throttlingPolicy) or Number(...), returns -1 when NaN or the numeric value
otherwise, and replace both inline IIFEs with a call to this helper; update
references in the mapping that set throttlingLimit.requestCount and the earlier
use (around the code that previously had the same Number(...) parsing) to call
the new helper (keep helper name unique to find it easily).
- Line 363: The assignment resiliency: Number(state.timeout) || undefined will
coerce a valid 0 to undefined; update the resiliency calculation to use an
explicit finite-number check consistent with other code (e.g., use
Number.isFinite on the parsed value and ensure > 0 if that’s desired) so that
resiliency is set only when the timeout parses to a valid number; locate the
resiliency property assignment in ManageDrawer (and reference validateTimeout to
keep behavior consistent) and replace the || undefined pattern with the
explicit-NaN/finite check used elsewhere.
In `@ipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsx`:
- Around line 162-166: handleClose currently schedules resetLocalValueMap via
setTimeout and doesn't cancel it, which can run after unmount and cause state
updates on an unmounted component; fix by storing the timeout id when calling
setTimeout in handleClose (e.g., let timeoutId = window.setTimeout(...)) and
clear it with clearTimeout(timeoutId) before creating a new timeout and in a
useEffect cleanup that runs on unmount or when valueMap/validationMap change;
ensure references to handleClose, resetLocalValueMap, valueMap, validationMap
and the stored timeout id are used so the pending timeout is always cancelled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afc8abac-b4d6-461e-b50b-ce77563de35a
📒 Files selected for processing (8)
ipaas/src/components/Deploy/EndpointConfigDrawer.tsxipaas/src/components/EnvironmentCard/ConfigureDrawer.tsxipaas/src/components/EnvironmentCard/EndpointCard.tsxipaas/src/components/EnvironmentCard/ManageDrawer.tsxipaas/src/components/SchemaConfigForm/FormElements/ArrayElement.tsxipaas/src/components/SchemaConfigForm/FormElements/ConfigElement.tsxipaas/src/components/SchemaConfigForm/tomlUtils.tsipaas/src/pages/BrowseSamples.tsx
✅ Files skipped from review due to trivial changes (3)
- ipaas/src/components/SchemaConfigForm/FormElements/ConfigElement.tsx
- ipaas/src/components/EnvironmentCard/EndpointCard.tsx
- ipaas/src/components/Deploy/EndpointConfigDrawer.tsx
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning